Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(colorwheel): S2 migration #3390

Open
wants to merge 3 commits into
base: spectrum-two
Choose a base branch
from

Conversation

cdransf
Copy link
Member

@cdransf cdransf commented Nov 12, 2024

Description

CSS-1020

This change migrates the colorwheel component to S2. It adds the --spectrum-colorwheel-border-color-rgb and --spectrum-colorwheel-border-opacity custom properties. It updates --spectrum-colorwheel-border-color to leverage these tokens in an rgba(...) function.

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

  1. Run Storybook locally or open the link for the PR.
  2. Navigate to the colorwheel component.
  3. Verify that no regressions are visible.

Regression testing

Validate:

  1. The documentation pages for at least two other components are still loading, including:
  • The pages render correctly, are accessible, and are responsive.
  1. If components have been modified, VRTs have been run on this branch:
  • VRTs have been run and looked at.
  • Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

To-do list

  • I have read the contribution guidelines.
  • I have updated relevant storybook stories and templates.
  • I have tested these changes in Windows High Contrast mode.
  • If my change impacts other components, I have tested to make sure they don't break.
  • If my change impacts documentation, I have updated the documentation accordingly.
  • ✨ This pull request is ready to merge. ✨

@cdransf cdransf added size-2 S ~6-18hrs; not hard or time consuming, one or two work days to complete. skip_vrt Add to a PR to skip running VRT (but still pass the action) ready-for-review labels Nov 12, 2024
@cdransf cdransf self-assigned this Nov 12, 2024
Copy link

changeset-bot bot commented Nov 12, 2024

🦋 Changeset detected

Latest commit: 87b66e7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@spectrum-css/colorwheel Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Nov 12, 2024

🚀 Deployed on https://pr-3390--spectrum-css.netlify.app

Copy link
Contributor

github-actions bot commented Nov 12, 2024

File metrics

Summary

Total size: 1.42 MB*
Total change (Δ): 🔴 ⬆ 1.21 KB (0.08%)

Table reports on changes to a package's main file.

Package Size Minified Gzipped Δ
colorwheel 5.90 KB - - 🔴 ⬆ 1.21 KB

colorwheel

Filename Head Minified Gzipped Compared to base
index.css 5.90 KB - - 🔴 ⬆ 1.21 KB
metadata.json 2.05 KB - - 🔴 ⬆ 0.14 KB
* Size is the sum of all main files for packages in the library.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

@cdransf cdransf changed the title feat(colorwheel): s2 migration feat(colorwheel): S2 migration Nov 12, 2024
Copy link
Collaborator

@marissahuysentruyt marissahuysentruyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions for you, as usual!

  1. I wonder if we need to refactor the color-wheel-width and color-wheel-minimum-width tokens from our custom tokens/dist. Those values change based on the platform (desktop/mobile), but the designs don't say anything about any size changes between desktop and mobile. In other components, they have noted the size differences. It might be something worth checking with design on. 🤷‍♀️
  2. Do we need to adjust .spectrum-ColorWheel-border at all so that our border is actually transparent, against the color wheel, instead of outside of the color wheel? I'm not sure if the clip path is the right place, but maybe that custom property needs some tweaking? I think S2 would be the place to fix this! (because it's also not like that in main, but looking at S1 designs, it should have been).

Ours (effectively, has a gray border):
Screenshot 2024-11-13 at 4 17 58 PM

In Figma (where the border is "on top" of the color wheel):
Screenshot 2024-11-13 at 4 18 19 PM

Happy to pair on any of my comments if two heads are better than one! 👍

components/colorwheel/index.css Show resolved Hide resolved
components/colorwheel/index.css Show resolved Hide resolved
components/colorwheel/index.css Outdated Show resolved Hide resolved
components/colorwheel/index.css Show resolved Hide resolved
@cdransf
Copy link
Member Author

cdransf commented Nov 14, 2024

A few questions for you, as usual!

  1. I wonder if we need to refactor the color-wheel-width and color-wheel-minimum-width tokens from our custom tokens/dist. Those values change based on the platform (desktop/mobile), but the designs don't say anything about any size changes between desktop and mobile. In other components, they have noted the size differences. It might be something worth checking with design on. 🤷‍♀️
  2. Do we need to adjust .spectrum-ColorWheel-border at all so that our border is actually transparent, against the color wheel, instead of outside of the color wheel? I'm not sure if the clip path is the right place, but maybe that custom property needs some tweaking? I think S2 would be the place to fix this! (because it's also not like that in main, but looking at S1 designs, it should have been).

Ours (effectively, has a gray border):

In Figma (where the border is "on top" of the color wheel): ...

Happy to pair on any of my comments if two heads are better than one! 👍

I started a thread in the implementations channel about the first question — I'll go ahead and run with design's preference.

Since the border is rendered by the clip path but is associated with the div that wraps the color wheel node, I'm not sure we can fix it by adjusting the clip path (to your point). If we reduce the dimensions of the div or reduce the size of the clip path the "border" would disappear since it would render behind the color wheel. Could we render an inset border and attach it to the color wheel node directly? (Pending design approval and input.) ✨

@cdransf cdransf force-pushed the cdransf/s2-colorwheel-migration branch from eaf2611 to 3a12421 Compare November 14, 2024 00:15
@cdransf cdransf added the blocked See description and comments for what is blocking this issue label Nov 14, 2024
@cdransf cdransf force-pushed the cdransf/s2-colorwheel-migration branch 2 times, most recently from 99a6ad6 to f3be898 Compare November 20, 2024 22:30
@cdransf cdransf removed the blocked See description and comments for what is blocking this issue label Nov 26, 2024
@castastrophe castastrophe force-pushed the spectrum-two branch 3 times, most recently from cdb180d to 27d01df Compare December 4, 2024 14:54
@cdransf cdransf force-pushed the cdransf/s2-colorwheel-migration branch 2 times, most recently from 7ce5746 to 7af66a2 Compare December 10, 2024 15:18
@cdransf cdransf force-pushed the cdransf/s2-colorwheel-migration branch 3 times, most recently from 9cb357e to 70603e0 Compare December 19, 2024 22:38
@castastrophe castastrophe force-pushed the spectrum-two branch 2 times, most recently from c93daf7 to e393c32 Compare December 27, 2024 18:11
@cdransf cdransf force-pushed the cdransf/s2-colorwheel-migration branch 2 times, most recently from a984eb6 to 61d7526 Compare February 4, 2025 22:12
@castastrophe castastrophe force-pushed the spectrum-two branch 2 times, most recently from 56c6806 to 793571c Compare February 5, 2025 17:33
@cdransf cdransf force-pushed the cdransf/s2-colorwheel-migration branch from 1f3d572 to eb8c57f Compare February 5, 2025 19:20
@cdransf cdransf force-pushed the cdransf/s2-colorwheel-migration branch from eb8c57f to 2454e32 Compare February 7, 2025 00:44
@castastrophe castastrophe force-pushed the spectrum-two branch 2 times, most recently from db2a5dd to f34473b Compare February 7, 2025 17:35
@cdransf cdransf force-pushed the cdransf/s2-colorwheel-migration branch 3 times, most recently from 1bfefa5 to ee52db2 Compare February 7, 2025 17:42
@cdransf cdransf force-pushed the cdransf/s2-colorwheel-migration branch from ee52db2 to 02a3f15 Compare February 7, 2025 21:12
--spectrum-colorwheel-height: var(--mod-colorwheel-height, var(--spectrum-color-wheel-width));
--spectrum-colorwheel-fill-color-disabled: var(--mod-colorwheel-fill-color-disabled, var(--spectrum-disabled-background-color));
--spectrum-colorwheel-border-color: var(--spectrum-transparent-black-300);
--spectrum-colorwheel-colorarea-margin: var(--spectrum-color-wheel-color-area-margin);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed while inspecting that the way we're using this colorarea margin is not how design has it defined on the specs. I don't think there is a way for us to use the token properly, as it appears to be relying on circle based calculations by the implementation. If I change --mod-colorarea-height and --mod-colorarea-width from 80px to 100%:
Screenshot 2025-02-10 at 11 04 01 AM

Design currently:
Screenshot 2025-02-10 at 10 57 07 AM

I'm wondering if we should even be setting this margin, if we can't calculate the actual value in CSS. Does this token value just need to be used when calculating the size of the color area? That "With color area" section of the docs is a little confusing. Perhaps this needs more documentation too. I don't see a SWC example with the color area within the color wheel?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good catch! I don't think we need to — I removed the custom property and the declaration and no difference is made in how it's displayed. It's positioning is controlled by...

.spectrum-ColorWheel-colorarea-container {
    block-size: auto;
    inline-size: 100%;
    display: flex;
    align-items: center;
    justify-content: center;
}

...and the margin has no impact.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to just document that the --spectrum-color-wheel-color-area-margin token used for that margin should be factored in by the implementation when calculating the size of the color area. Otherwise it could easily be forgotten about.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a comment above the .spectrum-ColorWheel-colorarea-container class in e1331f explaining the token (as it had previously been applied there).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the large platform scale, the inner border is no longer at the edge (Chrome):
Screenshot 2025-02-10 at 11 25 36 AM

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I was afraid this would happen — should we consider removing and provide design with the feedback that it's not possible at the moment? @marissahuysentruyt

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the styles here so that they're a bit more stable and based on the width of the inner, colorarea container which resolves the issue in large but causes the medium size to display within the interior of the color wheel instead of on top of the interior edge: 35403c.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All sorted thanks to @marissahuysentruyt ✨🚀

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

michael-scott-disco-coffee

Comment on lines 26 to 31
"--mod-colorwheel-height",
"--mod-colorwheel-min-width",
"--mod-colorwheel-min-inline-size",
"--mod-colorwheel-path",
"--mod-colorwheel-path-borders",
"--mod-colorwheel-track-width",
"--mod-colorwheel-width"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we still be shipping height and width mods? I see they don't really work on their own in CSS in prod, but I do see that the size of the color wheel should be customizable (https://opensource.adobe.com/spectrum-web-components/components/color-wheel/#sized).

Is there any way to allow a custom sizing example or are we excluding this because we'd need to dynamically calculate the clip-path? Are the clip path calculations not possible with CSS, or is that something that veers too far into implementation to do in our JS? Alternatively, I wonder if we could have a differently sized version in docs/VRT that shows that this still works, even if it's a hardcoded clip path?

It does seem like the implementation requirement for clip path calculation needs to be added to the component Docs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure — the React docs allow for the same dimensions we're seeing here (they link here) and the token that's being shipped provides the same dimensions: https://github.com/adobe/spectrum-tokens/blob/93614f4c98b5bc8ca1c017934ddfc456699a30c7/packages/tokens/src/layout-component.json#L4654

image

Copy link
Collaborator

@jawinn jawinn Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are the default sizes, but the size can still be customized (as longer as it's greater than or equal to the minimum size), as mentioned in the guideline. I don't think we necessarily need to generate the clip-path calculation at this time.

Mods: There should still be a mod wherever --spectrum-colorwheel-width and --spectrum-colorwheel-height are used. Unless there was some reason you were getting rid of them; it's especially important when the size of the color slider is being changed since there is other CSS calculating off these values. Also what about --mod-colorwheel-track-width and should there be a --mod-colorwheel-border-width?

Also, I see you renamed min-width to min-inline-size; should the same logical property renaming be done to the width and height tokens to be consistent?

Docs: Could you add something about the implementation requirement for clip path calculation when changing the size?

New custom sizing story?: What if we add a custom sizing story at 300x300? We can use the mod for the clip path with the value being generated in SWC's sizing example. The docs about the clip path calculation could be included in the story description here. This would help ensure in our VRTs that this use case is still working.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs: Speaking of the "With color area" section of the docs, it looks like some of the existing docs are incorrect. I see mention of a couple classes that I think were meant to reference a custom property instead. See ".spectrum-colorwheel-colorarea-container-size", ".spectrum-color-wheel-color-area-margin", ".spectrum-colorwheel-path", etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran through and cleaned these up — some are classes, some are custom properties and some have been removed altogether. ✨

@cdransf cdransf added blocked See description and comments for what is blocking this issue and removed blocked See description and comments for what is blocking this issue labels Feb 10, 2025
@cdransf cdransf force-pushed the cdransf/s2-colorwheel-migration branch from 1dce4dd to 71e76e1 Compare February 11, 2025 14:42
@castastrophe castastrophe force-pushed the spectrum-two branch 2 times, most recently from a604c1d to 4afa4a9 Compare February 11, 2025 18:20
@cdransf cdransf force-pushed the cdransf/s2-colorwheel-migration branch 3 times, most recently from 09e6366 to c5ef99c Compare February 11, 2025 18:41
@castastrophe castastrophe force-pushed the spectrum-two branch 2 times, most recently from dee81f5 to ce2f3e0 Compare February 11, 2025 19:53
- chore(colorwheel): update property name
- chore(colorwheel): wip inset borders
- chore(colorwheel): fix lint violations
- chore(colorwheel): move mod declarations
- chore(colorwheel): drop unused property + update metadata
- chore(colorwheel): remove underlying border node + class
- chore(colorwheel): remove unused property
- fix(colorwheel): add missing mod
- fix(colorwheel): comments for updated before spacing/positioning
- fix(colorwheel): adopt proper token for border opacity
- fix(colorwheel): remove inset border on disabled state
- fix(colorwheel): remove global token reference
- chore(colorwheel): highlight removed/added mods
- chore(colorwheel): update copyright year
- chore(colorwheel): add color loupe to default story
- fix(colorwheel): correct WHC disabled background color value
- chore(colorwheel): update test heading
- fix(colorwheel): typos
- chore(colorwheel): update token usage
- chore(colorwheel): add without loupe test
- chore(colorwheel): restore WHC styles
- chore(colorwheel): move isWithColorLoupe property
- fix(colorwheel): whc disabled background
- fix(colorwheel): improve interior border styles
- fix(colorwheel): token/classnames in story; remove unnecessary margin
- chore(colorwheel): fix inset borders + improve comments
- chore(colorwheel): update changeset
@cdransf cdransf force-pushed the cdransf/s2-colorwheel-migration branch from c5ef99c to 2a52b0c Compare February 11, 2025 20:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review S2 Spectrum 2 size-2 S ~6-18hrs; not hard or time consuming, one or two work days to complete. skip_vrt Add to a PR to skip running VRT (but still pass the action)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants